Repository navigation
Conversation
ilobko
changed the title
c05dee9 to
84e4e1d
Compare
ilobko
changed the title
t.uuid and add_uuid_column passed the pre-1.0 column_name: option straight to Active Record. Rails 7.1 and later reject it. Earlier versions ignored it and created a column named uuid. Use column_name: when no positional name is given, and raise an error when the two names differ.
44c74ed to
a1ee287
Compare
ilobko
changed the title
No spec used a database, so the UUID types and the v1 conversion helpers were never tested against MySQL. Add specs for how models and the v1 conversion helpers behave today, so the following changes can show that they keep this behavior. They are tagged :mysql and run only when UUIDABLE_DATABASE_URL points to a MySQL database ending in _test. gemfiles/activerecord.gemfile runs them against a chosen Active Record version; the README explains how. Remove the unused travis dependency: on Ruby 3.4 it pulls in a json_pure version that fails to load. Set RuboCop's target Ruby explicitly; in a clean checkout it fell back to 2.6.
uuidable read the table's columns while the model class was being defined. An app that loads all classes at boot could not start while MySQL was unreachable. A model defined before its table existed read 16-byte UUIDs as raw bytes until the app restarted. Set the UUID column types when Active Record loads the model's schema instead. Types declared with `attribute` still take precedence.
Since 1.0, t.uuid and add_uuid_column create 16-byte columns. An app upgraded from 0.x still has 36-byte columns, so its next migration mixed the two formats, and joins between them matched nothing. Add a storage: option (:binary or :text) to both helpers, and Uuidable.default_storage for columns that set neither storage nor limit. An explicit limit still wins. A limit that contradicts the storage raises an error.
uuidable_migrate_uuid_columns_to_v1 ran four separate statements, so a failure could leave a table half converted. For example, two UUIDs that differed only in case left the table without indexes. Writes made during the conversion were lost, index options and column comments were dropped, and a rollback could restore outdated values. Convert with two ALTER TABLE statements, each of which MySQL applies fully or not at all. The first adds the converted column as a generated column, so it keeps up with new writes. The second renames the original column to *__old, turns the generated column into a regular one, and rebuilds the indexes with their options. Also: - refuse columns that are part of a primary or foreign key; - refuse a rollback when *__old is missing later changes, and skip columns that were created with 16 bytes; - make migrate and rollback each other's inverse, so both work inside change; - keep a column's nullability unless its options override it; - drop only UUID *__old columns in uuidable_drop_all_pre_v1_uuid_columns!; - document the helpers in the README.
a1ee287 to
3a29a96
Compare
ilobko
changed the title
gsmetal
left a comment
gsmetal
left a comment
There was a problem hiding this comment.
Checked locally against MySQL 8.4.5: all 80 specs pass on Active Record 7.1 and 8.1; 6.1 passes once mutex_m/bigdecimal/base64/drb are added for Ruby 3.4. RuboCop is clean. Findings are in the threads below.
| def load_schema! | ||
| super | ||
| install_uuid_types unless SchemaTypes.type_for_column_hook? | ||
| include V1ModelMigration if columns_hash.each_key.any? { |name| name.include?(V1MigrationHelpers::OLD_POSTFIX) } |
There was a problem hiding this comment.
Should fix: V1ModelMigration is included whenever any column name contains __old, not only UUID columns. Its before_validation then overwrites every *__old attribute with the value of the same-named column without the suffix: with a notes__old = 'legacy' column next to notes, a save! turns it into 'edited'. uuidable_drop_all_pre_v1_uuid_columns! was tightened to include?('uuid') in this diff; the same guard belongs here and in the callback's attribute loop.
| end | ||
|
|
||
| # 16-byte values are already binary. Anything else that is not a UUID makes UUID_TO_BIN fail the ALTER. | ||
| def converted_value(column_name) |
There was a problem hiding this comment.
Should fix: for a nullable column the transition callback stores '' in *__old instead of NULL (nil.to_s). After the first save of such a row the rollback is impossible: refuse_stale_old_values! fails with Mysql2::Error: Incorrect string value: '' for function uuid_to_bin rather than V1Conversion::Error, so the README's rollback guarantee does not hold for nullable columns. NULLIF(col, '') inside converted_value plus writing nil instead of '' in the callback fixes both sides.
| if ENV['UUIDABLE_DATABASE_URL'] | ||
| require_relative 'support/database' | ||
| else | ||
| RSpec.configure { |config| config.filter_run_excluding :mysql } |
There was a problem hiding this comment.
Should fix: none of the database-backed specs run in CI. .github/workflows/ruby.yml has no MySQL service and no UUIDABLE_DATABASE_URL, and its Ruby 2.7-3.1 matrix never uses gemfiles/activerecord.gemfile, so only the 28 pure-Ruby examples from migration_spec.rb execute. A regression in the conversion ALTERs or in the type_for_column / install_uuid_types paths would merge green. A services: mysql:8 block, the env var and a matrix over ACTIVE_RECORD_VERSION would cover it.
| eval_gemfile File.expand_path('../Gemfile', __dir__) | ||
|
|
||
| gem 'activerecord', ENV.fetch('ACTIVE_RECORD_VERSION') | ||
| gem 'mysql2', '~> 0.5' |
There was a problem hiding this comment.
Should fix: on Ruby 3.4 with Active Record 6.1 or 7.0 require 'active_record' fails with cannot load such file -- mutex_m, while the README says the suite runs against any supported version. Conditional mutex_m, bigdecimal, base64 and drb for AR < 7.1 make the gemfile work on current Ruby.
|
|
||
| private | ||
|
|
||
| def uuidable_convert_to_v1(table_name, columns_options, opts) |
There was a problem hiding this comment.
Nit: a columns_options key that matches no convertible column (a typo, or a varchar(36) column that fails valid_column_for_migration?) is silently skipped and the migration is recorded as applied. Diffing the requested names against the selected columns and raising on the difference would surface it.
| "IF(OCTET_LENGTH(#{column}) = 16, #{column}, UUID_TO_BIN(#{column}))" | ||
| end | ||
|
|
||
| def definition(column, options) |
There was a problem hiding this comment.
Nit: definition rebuilds the column from limit/null/comment only, so a DEFAULT on the original column is dropped. The rollback keeps it (it only renames), so up and down are not symmetric.
| end | ||
| end | ||
|
|
||
| def remove_converted_columns_on_failure(columns) |
There was a problem hiding this comment.
Nit: when both the second ALTER and the cleanup fail, a *__new stored generated column survives and no helper can see it (valid_column_for_migration? excludes NEW_POSTFIX); a retry fails with Duplicate column name. Dropping a leftover *__new before add_converted_columns, or raising a clear error, would make the conversion retryable.
|
|
||
| # Storage for new UUID columns that set neither `storage:` nor `limit:`. | ||
| # Apps upgrading from 0.x can set :text so new columns match their 36-byte ones. | ||
| def default_storage |
There was a problem hiding this comment.
Nit: default_storage is read when a migration runs, so the migration history stops being reproducible once the setting changes: a bare t.uuid gives 16 bytes in production and 36 bytes in a database rebuilt from scratch. The README should at least say the setting has to be in place before the first migration run on 1.x.
| spec.add_development_dependency 'rspec', '~> 3.0' | ||
| spec.add_development_dependency 'travis', '~> 1.8', '>= 1.8.2' | ||
|
|
||
| spec.add_dependency 'activerecord', '>= 4.2', '< 8.2' |
There was a problem hiding this comment.
Nit: the activerecord >= 4.2 lower bound no longer matches the code: define_attribute(user_provided_default:), attributes_to_define_after_schema_loads and t.virtual stored: appeared in 5.0-5.1.
2 participants
Apps on uuidable 0.x have 36-byte text UUID columns. This PR fixes the problems they hit when upgrading to 1.x, whether or not they convert those columns. It also adds specs that run against MySQL.
Upgrading without converting
t.uuid column_name: :xandadd_uuid_column :t, column_name: :xfailed on Rails 7.1+ withUnknown key: :column_name. They work again. Passing two different names now raises an error.uuidableno longer queries the database. Before, an app that loads all classes at boot couldn't start while MySQL was down.storage: :textorstorage: :binary, andUuidable.default_storage = :textmakes text the default.Converting
uuidable_migrate_uuid_columns_to_v1could leave a table half converted when something went wrong. For example, two UUIDs that differed only in case left the table without indexes. The conversion now runs as two statements, and MySQL applies each one fully or not at all. It also:change, so the migration can be rolled back.A rollback now refuses to throw away changes that the
*__oldcolumns don't have.uuidable_drop_all_pre_v1_uuid_columns!drops only UUID*__oldcolumns. Method names and the resulting schema stay the same.One behavior change to note:
{ 'col' => {} }now keeps the column's nullability. Before, it made the columnNOT NULL.Testing
The new specs run against MySQL when
UUIDABLE_DATABASE_URLis set and are skipped otherwise. The README explains how to run them with a chosen Active Record version. The unusedtravisdependency is removed, because it broke installs on Ruby 3.4.All 80 specs pass against MySQL 8.0.46 with Active Record 6.1, 7.0, 7.1, 7.2, 8.0 and 8.1. Each commit passes on its own, and RuboCop is clean.